I don't like all of this style of change. StataReader is self-closing. Hsa this changed? If so, that is introducing a new bug to user code that needs to be addressed.

@bashtage StataReader is not self-closing. It has had a close method since 59dd18b (July 2015), to, quote, "ensure closing of the path". (At that point, when you'd pass in a string-like to the ctor, it would open it as a regular file and not buffer things into memory.) You can see 59dd18b also changes some tests to use with.

Some tests either didn't use that, or didn't call close(), which in turn caused cleanup issues on Windows now that we don't buffer everything into memory (which was a regression in 6d1541e, 2020).

Any user code that had not used StataReader() as a context manager had technically been in violation of the protocol established in 59dd18b, it just wouldn't surface since Pandas used to read everything to memory for the last two years.

I have a strong preference for there to be no test changes aside from those essential for this PR, so please revert these. It is fine to do a follow-up PR to clean the test code to use best practices. This just ensures that there are no visible side effects of other changes, e.g., leaking handles with existing code.

Leaked handles should trigger a CI failure, and on Windows, open handles usually result in a failure to delete the file which is a test failure.

@bashtage

Leaked handles did CI fail, hence this separate commit to fix those up. See earlier discussion:

• REGR: be able to read Stata files without reading them fully into memory #48922 (comment)
• REGR: be able to read Stata files without reading them fully into memory #48922 (comment)

This behavior arose from a human mistake in the refactoring in 6d1541e.

You call it a mistake, I call it a coding-efficient choice that was not memory efficient.

I don't think it's first and foremost an optimization as the current behavior entirely prevents using chunked or iterator reading on machines with less available memory than the stata file's size on disk.

The usual response is to get more memory. I think extending the reading to lower memory machines is an enhancement.

I don't think that's true. The test suite changes (where a block already ending with .close() wasn't changed to a with) are:

Most of these self-close. There is only one that would need an explicit call to close if you implement this change only for the case where an iterator is used. (iterator == True or chunksize not None).

Reading a data label and variable labels only, no iterator mode:

It is self-closing in main, so no leaks.

Chunked Stata reader left open:

It is not. Closed since iterator is exhausted.

Chunked Stata reader left open.

Yes. This one could be fixed in the test.

Only value labels being read, no iterator mode (this occurs three times with minor variations):

Should work without modification of the tests since not an iterator.

To move this forwards, why not try:

• implement it only in the case where an iterator is indicated
• Restore the test suite so we can see the failures
• Leave in the new tests, but make adapt them so that they are using an iterator

You call it a mistake, I call it a coding-efficient choice that was not memory efficient.

Please recall the reading code before that inadvertent change was memory-efficient.

The usual response is to get more memory.

Which is quite user-hostile when in this case the situation can be fixed with a +61/-26 line diff. (On that note, the person who had the issue with a 30-some-gigabyte Stata file on Stack Overflow reached out to me on Twitter for help, and I suggested trying to patch their Pandas' stata.py with the one from this branch. I think that's friendlier and quite less expensive (monetarily, ecologically, etc.) than to get more memory.)

I think extending the reading to lower memory machines is an enhancement.

See above. Restoring being able to read large files on lower memory machines is a regression fix or a bug fix.

It is self-closing in main, so no leaks.

On main, StataReader buffers to a BytesIO and immediately closes the original handle in the constructor. If that's your definition of "self-closing", then I'm not sure how to interpret your other arguments, since all use of StataReader on main (or rather, since 6d1541e) is "self-closing".

It is not. Closed since iterator is exhausted.

Which is technically a bug in itself, see #48922 (comment)

Yes. This one could be fixed in the test.

I'm not following you – why can this one be fixed in tests?

Should work without modification of the tests since not an iterator.

It only works on main without a leak because StataReader on main always buffers into memory, and an unclosed BytesIO does not raise a resource warning. It wouldn't work on any version prior to 6d1541e without a leak occurring. All of these three were added in fd151ba when the inadvertent buffering code was already in.

On main, StataReader buffers to a BytesIO and immediately closes the original handle in the constructor. If that's your definition of "self-closing", then I'm not sure how to interpret your other arguments, since all use of StataReader on main (or rather, since 6d1541e) is "self-closing".

The is exactly my point. The current behavior of StataReader is to never leak file handles. It has been this way for 2 years now. This is why the test suite passes despite some questionable code. Ideally, this behavior should not be changed without a deprecation cycle. It is probably necessary to change it to get the iterator to work without buffering the entire file, so I think it is acceptable to make this change in this limited case. I would also think the docs for iterator and chunksize should be strengthened to tell users that they must use a context manager or close the file themselves.

I do not think it is OK to change it where it isn't essential without letting users know of a change that could lead to errors.

I think it would be best to focus the fix on #48700 only and not expand the fix to apply to other cases.

The iterator mode is already self-closing. Why is the context manager introduced here?

Sure, in this test we're reading the reader through, but if we weren't, then it would be good form to close the reader (and the underlying file handle), just like with any file handles in Python.

In fact, you might argue the self.close() there in the reader is inappropriate, since an esoteric user could have e.g. saved the reader's underlying file handle's .seek() position at some point, would then rewind the handle and have the reader read some more... but perhaps that's an esoteric enough use case that it doesn't need to be covered. 😁

Leaked handles should trigger a CI failure, and on Windows, open handles usually result in a failure to delete the file which is a test failure.